Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type-hint contract instead of concrete class to allow switching features #66

Closed

Conversation

brunodevel
Copy link

@brunodevel brunodevel commented May 24, 2024

Changes In Code

When we fake a feature via the Features::fake method, the manager instance is swapped with FeatureFake.

Calling fake more than once will result in the following error:

YlsIdeas\FeatureFlags\Support\FeatureFake::__construct(): Argument #1 ($manager) must be of type YlsIdeas\FeatureFlags\Manager, YlsIdeas\FeatureFlags\Support\FeatureFake given [...]

This happens because FeatureFake type-hints at a Manager instance but after Features::fake has been called for the first time, the facade root will return an instance of FeatureFake instead.

To fix that, I changed the type expected for the $manager property to the Features contract.

Issue ticket number / Business Case

Switching a feature state can be useful in tests.

In my specific case, I noticed the issue when I faked a feature in a test setUp but had one specific test case where the feature had to be switched, resulting in the error.

Checklist before requesting a review

  • I have written PHPUnit tests.
  • I have updated the documentation and opened a pull request within
    the feature flags documentation repo.
  • I have checked code styles, PHPStan etc. pass.
  • I have provided an issue/business case.
  • I have added the enhancement label for a new feature.

@brunodevel brunodevel requested a review from peterfox as a code owner May 24, 2024 08:20
@peterfox peterfox mentioned this pull request May 25, 2024
5 tasks
@peterfox
Copy link
Collaborator

Hi @brunodevel thanks for the PR and finding this issue. I really appreciate it when people contribute.

I've made my own PR for this that works more in line with how Laravel usually handles this situation. I'd love it if you could check it out and verify it solves the issue for you?

@brunodevel
Copy link
Author

Hey @peterfox, looks good to me.
Thank you.

@brunodevel brunodevel closed this May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants